BUG: Fix __ne__ comparison for Categorical #32304
Conversation
pandas/core/arrays/categorical.py
Outdated
| @@ -103,7 +103,10 @@ def func(self, other): | |||
| mask = (self._codes == -1) | (other_codes == -1) | |||
| if mask.any(): | |||
| # In other series, the leads to False, so do that here too | |||
| ret[mask] = False | |||
| if opname == "__ne__": | |||
| ret[mask & (self._codes == other_codes)] = True | |||
There was a problem hiding this comment.
This redundancy was intentional and an attempt to "reuse" the mask calculation above since I think it may short-circuit when mask is False, but could just make it (self._codes == -1) & (other_codes == -1) as well. I also think __ne__ is the only situation that'd have to be special-cased here out of the comparison operators. Also worth pointing out that this is assuming we're dealing with NaN rather than NA.
There was a problem hiding this comment.
(self._codes == -1) & (other_codes == -1)
I think this would be more clear. Also there might be a cached _isnan attribute for this
There was a problem hiding this comment.
@dsaxton im working on cleaning up these comparisons, am confused by this line. can we make this use the more common pattern
fill_value = True if op is operator.ne else False
[...]
ret = op(self._codes, other_codes)
mask = (self._codes == -1) | (other_codes == -1)
ret[mask] = fill_value
There was a problem hiding this comment.
@jbrockmendel That is much simpler and seems correct to me
| @@ -282,6 +282,16 @@ def _compare_other(self, s, data, op_name, other): | |||
| with pytest.raises(TypeError, match=msg): | |||
| op(data, other) | |||
|
|
|||
| def test_not_equal_with_na(self): | |||
There was a problem hiding this comment.
do we get this right in cases with e.g. datetime64 or datetime64tz categories?
There was a problem hiding this comment.
It seems to work, added some parameterization over other category types
jreback
added
Categorical
|
thanks @dsaxton |
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diff